-
Notifications
You must be signed in to change notification settings - Fork 5
test: Run tutorials in tests #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| print("Setting Q_damp and Q_broad to the refined values\n") | ||
| QDAMP_I = 0.045298 | ||
| QBROAD_I = 0.016809 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user doesn't run the bulkNi script, the Qdamp and Qbroad values will be defined here. This is also needed to run tests (for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's see if we can fix this properly and not this ugly workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge To handle this, I had to add special handling for this script. I've pushed said special handling. Let me know if that looks okay.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
===========================================
+ Coverage 50.00% 81.75% +31.75%
===========================================
Files 2 11 +9
Lines 18 1211 +1193
===========================================
+ Hits 9 990 +981
- Misses 9 221 +212
🚀 New features to boost your workflow:
|
|
@sbillinge ready for review. Thanks for your help on this. I think this is nice and clean and it looks like the temp dir gets removed. Currently looking into ways of initializing the refinements with the refined values so this test runs quicker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls see comments
|
|
||
| # Change some style details of the plot | ||
| mpl.rcParams.update(mpl.rcParamsDefault) | ||
| if (PWD.parent.parent.parent / "utils" / "billinge.mplstyle").exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the group style added back in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge I will have to add it to each file I think, I can do this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better on another PR, but can we also do this work on the examples scripts also? Not just the solutions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge Yeah I can do this. Do you want me to revert back then?
|
|
||
| print("Setting Q_damp and Q_broad to the refined values\n") | ||
| QDAMP_I = 0.045298 | ||
| QBROAD_I = 0.016809 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's see if we can fix this properly and not this ugly workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls see comments
tests/test_examples.py
Outdated
| scripts = list(tmp_examples.rglob("**/solutions/diffpy-cmi/*.py")) | ||
| """Run all example scripts to ensure they execute without error.""" | ||
| # Run Ni example first to produce .res file | ||
| ni_script = list(tmp_examples.rglob("**/FitBulkNi.py"))[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have turned something beautiful into something very ugly though. I think the figbulkNI runs first anyway so we probably don't need any of this. Do you want to have another crack at it, or should I try and see what I can come up with?
Also why are you recursively globbing a single file? I can see we need to work a bit on your coding skills......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge Let me give this another shot. Im just trying to figure out how to pull the results file from the temp dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice progress. Please see my comment. Looking much more in the right direction though..... I will wait for your next iteration.
|
|
||
| # Change some style details of the plot | ||
| mpl.rcParams.update(mpl.rcParamsDefault) | ||
| if (PWD.parent.parent.parent / "utils" / "billinge.mplstyle").exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better on another PR, but can we also do this work on the examples scripts also? Not just the solutions?
|
@sbillinge I figured out whats going on. |
|
@sbillinge ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one last thing.
| # sort list so that fitBulkNi.py runs first | ||
| scripts.sort(key=lambda s: 0 if s.name == "fitBulkNi.py" else 1) | ||
| for script in scripts: | ||
| script_relative_path = script.relative_to(tmp_examples).as_posix() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is your reason for the posix? this is likely to make it not work on windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge This is to make it compatible across operating systems when converting the path to a string. Maybe it can go either way here (use str() or posix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just delete it tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Will do
|
@sbillinge responded to your comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
respond to comment
| # sort list so that fitBulkNi.py runs first | ||
| scripts.sort(key=lambda s: 0 if s.name == "fitBulkNi.py" else 1) | ||
| for script in scripts: | ||
| script_relative_path = script.relative_to(tmp_examples).as_posix() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just delete it tbh
|
@sbillinge Made the change. This test should pass in ~20 min. |
closes #8